Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(50375): Errors for missing enum-named properties should attempt to preserve names #50382

Merged
merged 6 commits into from
Aug 24, 2022

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #50375

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Aug 20, 2022
src/compiler/checker.ts Outdated Show resolved Hide resolved
Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new method seems better, but the flags don't look right. Maybe rewrite them as shifts like the others flags?

src/compiler/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're running out of flags - does it make sense to have merge any of these flags at all?

src/services/codefixes/fixAddMissingMember.ts Outdated Show resolved Hide resolved
@a-tarasyuk
Copy link
Contributor Author

We're running out of flags - does it make sense to have merge any of these flags at all?

Maybe that makes sense, however, I'm not sure which options can be combined.

@a-tarasyuk a-tarasyuk closed this Aug 23, 2022
@jakebailey
Copy link
Member

We're running out of flags - does it make sense to have merge any of these flags at all?

I'm unfortunately not seeing anything we could do to make this better; I think it's the case that we can use up to the full 32-bit range, so it's not a problem now, but...

@DanielRosenwasser
Copy link
Member

I think it's the case that we can use up to the full 32-bit range, so it's not a problem now, but...

I believe that for 64-bit versions of Node.js that's the case; but 64-bit Electron may have different rules due to its pointer compression encoding (see microsoft/vscode#151245). If I've read this V8 blog post correctly, under that mode an SMI doesn't get a full 32 bits.

src/compiler/types.ts Outdated Show resolved Hide resolved
@jakebailey
Copy link
Member

If Electron behaves differently, maybe we should be testing it; I bet it'd be straightforward to create a builder that does an ELECTRON_RUN_AS_NODE=1 or something (and even use @vscode/test-electron to grab the latest VS Code and run it).

@a-tarasyuk a-tarasyuk requested review from sheetalkamat and weswigham and removed request for jakebailey, sheetalkamat and weswigham August 24, 2022 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors for missing enum-named properties should attempt to preserve names
7 participants